Implement v1 file format #299
Conversation
cschwan
changed the title
This is necessary to counteract a recent change in `cargo-c`
The feature `static` should not be used when APPLgrid has been compiled with ROOT support, because ROOT doesn't support static linking
Co-authored-by: Felix Hekhorn <[email protected]>
|
|
||
| /// TODO | ||
| #[must_use] | ||
| pub fn fill_index(&self, value: f64) -> Option<usize> { |
There was a problem hiding this comment.
this prefix is a bit confusing, i.e. fill_index and fill_limits are not similar to each other - maybe insert_at_index?
There was a problem hiding this comment.
or actually, just insert?
There was a problem hiding this comment.
They are similar to each other in that they both refer to Grid::fill. This method has a parameter observable which is an f64, which is being checked if it falls inside the limits returned by BinsWithFillLimits::fill_limits. If that's not the case, BinsWithFillLimits::fill_index returns None, else Some(index) where index is the result. Of course, this should be part of the documentation that is still missing.
There was a problem hiding this comment.
okay, I misunderstood (so yes documentation) ... mmm I still think it's not a perfect name (as usual this is a subjective thing); how about search_fill_index? if you disagree please just resolve this comment
|
|
||
| /// Error type returned by [`BinRemapper::from_str`] | ||
| #[derive(Debug, Error)] | ||
| pub enum Bla { |
There was a problem hiding this comment.
| pub enum Bla { | |
| pub enum BinRemapperStrError { |
nur um das klar zu stellen: mein Standardname is 50% Bla und 50% Blub 🙈
There was a problem hiding this comment.
Yes, that still has to be changed.
| @@ -124,17 +124,17 @@ bitflags! { | |||
| #[derive(Clone, Deserialize, Serialize)] | |||
| pub struct Grid { | |||
| subgrids: Array3<SubgridEnum>, | |||
| channels: Vec<Channel>, | |||
| bin_limits: BinLimits, | |||
| bwfl: BinsWithFillLimits, | |||
There was a problem hiding this comment.
Can't we call it just bins here please? We don't care about the implementation details inside grid
There was a problem hiding this comment.
It's not an implementation detail. Every bin has effectively two sets of limits:
- The N-dimensional actual bin limits that also the user sees. These mustn't overlap, but they allow 'gaps' between them, also they must all have the same dimensions;
- Fill limits: these are one-dimensional limits, the 'fill limits', that only
Grid::filluses. These are as many limits as there are bins plus one more and all values must be ascending.
There was a problem hiding this comment.
That being said I suppose we can find a better name. I'd like to avoid Bins though, because this struct has a method fn bins() and then the code would have a lot of grid.bins().bins() invocations, which is a bit odd.
There was a problem hiding this comment.
That being said I suppose we can find a better name.
that was my main concern - bins_config?
| impl FromStr for BinsWithFillLimits { | ||
| type Err = Bla; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Bla> { |
There was a problem hiding this comment.
Is it possible to split this looong function into 4-5 smaller ones? naively it seems there is a clear path step 1 -> step 2 -> step 3 -> ... which act in isolation
There was a problem hiding this comment.
I can split this function into smaller ones, but I don't see that making it more readable. But let me try to improve it.
There was a problem hiding this comment.
to me it would be more readable - because then there will be a clear input, a clear task, and a clear output. Needless to say that this would also be good for unit testing.
There was a problem hiding this comment.
I agree that if this could be broken into smaller functions that'd be great as right now it is a bit difficult to read.
Feature TODOs:
Channelto support arbitrarily many PIDs. This is implemented with commits f32902a, 2417c92 and 03f2dc6.Ordershould support a fragmentation scale. Started with commits 5e281ae and 02e48e9.1(no scale variation),3(vary all scales with the same factor),7and9(assuming that there's no fragmentation scale), and17and27(assuming there's a fragmentation scale)PackedArraystruct that we wrote some time ago.v0PineAPPL grids and convert the subgrids into the new subgrid types.LHAPDFNAME+pfor polarized andLHAPDFNAME+ffor fragmentation function andLHAPDFNAME+pforLHAPDFNAME+fpfor bothCode TODOs:
Ordertou8channel!to arbitrarily many PIDsZEUS_2JET_319GEV_374PB-1_DIF_ETQ2_BIN6(available in the same location where also the other test data is)BinInfo, and have instead fill limits (1d limits that only concernGrid::fill) and bin limits (n-dimensional limits)Grid::evolve_infolagrange_subgridPackedQ1X2SubgridV1NodeValuesMu2D=Nforgeneral_tensor_mulinevolution.rsGrid::mergecheck if grids are compatible with each other before mergingSetType:EvolveInfo::frg1andFkTable::frg0@Radonirinaunimi Test remaining new features of PineAPPL 1.0 #325D=3forgeneral_tensor_mulinevolution.rs@Radonirinaunimi Test remaining new features of PineAPPL 1.0 #325CAPI TODO:
GridOptFlags(see commit 54a59f3)pineappl_channel_*topineappl_channels_*?-v1.cppsuffix of example programs to.cppand rename the previous example programs from.cppto-deprecated.cpp. This should clarify that the use ofv0functions is deprecated (but still works!)Fortran module TODO:
Python API:
pyo3-0.22.5, this possible now sincenumpy-0.22.0was released. Figure out what new features we can leverage from0.22.x(see its CHANGELOG):#![allow(unsafe_op_in_unsafe_fn)]3.7inpyproject.tomlGeneral:
CHANGLOG.mdentries fromv0.8.xbranch